rg/controller: use the default resource group if the requested one doesn't exist #8387
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8387 +/- ##
==========================================
- Coverage 77.30% 77.28% -0.02%
==========================================
Files 471 471
Lines 61679 61696 +17
==========================================
+ Hits 47679 47682 +3
- Misses 10413 10436 +23
+ Partials 3587 3578 -9
Flags with carried forward coverage won't be shown. Click here to find out more. |
Signed-off-by: JmPotato <[email protected]>
| @@ -356,22 +357,27 @@ func (c *ResourceGroupsController) Start(ctx context.Context) { | |||
| if err = proto.Unmarshal(item.Kv.Value, group); err != nil { | |||
| continue | |||
| } | |||
| if item, ok := c.groupsController.Load(group.Name); ok { | |||
| gc := item.(*groupCostController) | |||
| if gc, ok := c.loadGroupController(group.Name); ok { | |||
There was a problem hiding this comment.
Should we reset the tombstone flag in here if the user deletes it and creates it again?
| log.Warn("[resource group controller] resource group name does not exist", zap.String("name", resourceGroupName)) | ||
| return &rmpb.Consumption{}, nil |
There was a problem hiding this comment.
Do we need to advance the log and remove return?
There was a problem hiding this comment.
It's better not to do so since that may cause a lot of logs during the workload.
| @@ -402,8 +403,9 @@ func (suite *resourceManagerClientTestSuite) TestResourceGroupController() { | |||
| CPUMsCost: 1, | |||
| } | |||
|
|
|||
| controller, _ := controller.NewResourceGroupController(suite.ctx, 1, cli, cfg, controller.EnableSingleGroupByKeyspace()) | |||
There was a problem hiding this comment.
why remove EnableSingleGroupByKeyspace?
There was a problem hiding this comment.
Enabling it will prevent the controller from starting the watch channel, this test needs it.
Signed-off-by: JmPotato <[email protected]>
ti-chi-bot
bot
added
needs-1-more-lgtm
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HuSharp, nolouch The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@JmPotato: Your PR was out of date, I have automatically updated it for you. If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: close #8388.
What is changed and how does it work?
Check List
Tests
Release note